Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: remove symbol props from stubs #1086

Merged
merged 9 commits into from
Dec 13, 2021
Merged

fix: remove symbol props from stubs #1086

merged 9 commits into from
Dec 13, 2021

Conversation

lmiller1990
Copy link
Member

resolves #1076

More context: ionic-team/ionic-framework#24181

Basically when you do a stub that receives a Symbol as a prop, it ends up doing <blah-stub sym="Symbol()"> which isn't allowed. Try it in your browser: document.body.setAttribute('sym', Symbol()).

I just filter out all the symbol props. Bit of a hack, but not a breaking change (passing a symbol as prop to a stub causes an error in both VTU v1 and v2).

Happy to solicit any better work arounds for this issue, best I could come up with for now to unblock the issue in Ionic team repo.

@lmiller1990 lmiller1990 changed the title Issue 1076 fix: remove symbol props from stubs Nov 18, 2021
Copy link
Member

@cexbrayat cexbrayat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me think that the fix should not be in VTU but in Vue itself (as it won't be in webidl-conversion).
The same will happen in Vue if someone binds a Symbol to an attribute right?
So instead of having this dirty hack, I thhink the issue should be opened upstream on vue-next, and fix it there.

If we ever end up merging this hack in VTU, maybe we should consider not removing the Symbol, but replacing its value with a string equivalent, so the stub will properly reflect that there was a Symbol.

@cexbrayat
Copy link
Member

I opened a PR to fix this in vue-next instead vuejs/core#4964

If that gets merged we won't need this PR, and a simple upgrade to the latest vue-next will solve the issue.
Otherwise, we'll go with Lachlan's hack 😉

@lmiller1990
Copy link
Member Author

Nice, I added a comment to your PR vuejs/core#4964

I like your fix, although Vue core has so many open PRs - if it won't be merged for a while, we could use this until that PR is merged, then change to that implementation.

We should ping Evan for a review on that PR, as well as the jest.spyOn fix.

Copy link
Member

@cexbrayat cexbrayat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the PR in vue-next was not merged, let's go with the hacky patch in VTU 😉

src/stubs.ts Outdated
const $props = props as unknown as ComponentObjectPropsOptions
return Object.keys($props).reduce((acc, key) => {
if (typeof $props[key] === 'symbol') {
return acc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT of stringifying the Symbol instead of completely removing it? It would then show in the snapshot. (a simple $props[key].toString() should do the trick)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

given that this is only an issue when shallow mounting, I guess stringifying makes sense as people use shallow mounting to do things such as snapshotting or checking against a prop value. I never use it, but I guess stringifying the symbol aligns with the same mental model of "exploring" the rendered stuff

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

tests/props.spec.ts Outdated Show resolved Hide resolved
src/stubs.ts Outdated
@@ -62,7 +86,7 @@ export const createStub = ({
})
}

const createTransitionStub = ({ name }: StubOptions) => {
const createTransitionStub = ({ name, propsDeclaration }: StubOptions) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the changes to createTransitionStub necessary? I fail to see the link. If it is necessary maybe add a test case with a transition?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reverted

@lmiller1990
Copy link
Member Author

@cexbrayat @afontcu updated 👍

src/stubs.ts Outdated
@@ -42,6 +43,17 @@ const shouldNotStub = (type: ConcreteComponent) => doNotStubComponents.has(type)
export const addToDoNotStubComponents = (type: ConcreteComponent) =>
doNotStubComponents.add(type)

const removeSymbols = <T = ComponentPropsOptions>(props: T): T => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this could be renamed to stringifySymbols as it is not removing them anymore?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooc: is the generic type T necessary here? As it is only called once, can't we use ComponentPropsOptions directly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed that

src/stubs.ts Outdated
// something like `el.setAttribute('val', Symbol())` which is not valid and
// causes an error.
// Only a problem when shallow mounting. For this reason we iterate of the
// props that will be passed and remove any that are symbols.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

description is not accurate anymore: this should mention that they are stringified

src/stubs.ts Outdated
// causes an error.
// Only a problem when shallow mounting. For this reason we iterate of the
// props that will be passed and remove any that are symbols.
const propsWithoutSymbols = removeSymbols(ctx.$props)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: same here, the variable could be renamed

src/stubs.ts Outdated
@@ -164,7 +188,8 @@ export function stubComponents(
if (type === Transition && 'transition' in stubs && stubs['transition']) {
return [
createTransitionStub({
name: 'transition-stub'
name: 'transition-stub',
propsDeclaration: {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the option is reverted to optional, this can be removed.

src/stubs.ts Outdated
@@ -179,7 +204,8 @@ export function stubComponents(
) {
return [
createTransitionStub({
name: 'transition-group-stub'
name: 'transition-group-stub',
propsDeclaration: {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

src/stubs.ts Outdated
@@ -21,7 +22,7 @@ import { Stub, Stubs } from './types'

interface StubOptions {
name: string
propsDeclaration?: ComponentPropsOptions
propsDeclaration: ComponentPropsOptions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this necessary? Can't that be left optional, and have a default value in the stubs?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, fixed all comments, thanks for the review!

@cexbrayat cexbrayat merged commit 9ef6ad8 into master Dec 13, 2021
@cexbrayat cexbrayat deleted the issue-1076 branch December 13, 2021 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

using shallowMount on a component that uses Symbol causes tests to crash
3 participants